GODRIVER-3156 Detect and discard closed idle connections. #1815
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| // If the connection has been idle for less than 10 seconds, skip the liveness | ||
| // check. | ||
| idleStart, ok := c.idleStart.Load().(time.Time) | ||
| if !ok || idleStart.Add(10*time.Second).After(time.Now()) { |
There was a problem hiding this comment.
What is the significance of 10 seconds?
There was a problem hiding this comment.
My reference was the PyMongo driver, which checks connection liveness when it's idle for >1 second. I wanted to mitigate the risk that checking liveness too often would cause performance problems (since a liveness check takes at least 1ms), so I increased that threshold to 10s.
blink1073
added
priority-1-high
|
This looks relevant: |
API Change ReportNo changes found! |
prestonvasquez
previously approved these changes
Sep 30, 2024
There was a problem hiding this comment.
LGTM, I recommend adding a unit test for isAlive:
func TestConnection_IsAlive(t *testing.T) {
t.Run("idle time lt 10 seconds", func(t *testing.T) {
mockConn := &drivertest.ChannelNetConn{}
conn := &connection{
nc: mockConn,
}
conn.idleStart.Store(time.Now())
assert.True(t, conn.isAlive())
})
t.Run("idle time gt 10 seconds", func(t *testing.T) {
mockConn := &drivertest.ChannelNetConn{
ReadResp: make(chan []byte, 1),
}
mockConn.ReadResp <- []byte{5, 0, 0, 0, 0}
conn := &connection{
nc: mockConn,
}
conn.idleStart.Store(time.Now().Add(-11 * time.Second))
assert.False(t, conn.isAlive())
})
t.Run("read error", func(t *testing.T) {
mockConn := &drivertest.ChannelNetConn{
ReadErr: make(chan error, 1),
}
mockConn.ReadErr <- errors.New("")
conn := &connection{
nc: mockConn,
}
conn.idleStart.Store(time.Now().Add(-11 * time.Second))
assert.False(t, conn.isAlive())
})
// TODO:
// case: deadline exceeded
}
|
drivers-pr-bot please backport to release/1.17 |
prestonvasquez
approved these changes
Oct 1, 2024
mongodb-drivers-pr-bot bot
pushed a commit
that referenced
this pull request
Oct 1, 2024
Co-authored-by: Steven Silvester <[email protected]> (cherry picked from commit 6952473)
blink1073
pushed a commit
that referenced
this pull request
Oct 1, 2024
…lease/1.17] (#1841) Co-authored-by: Matt Dale <[email protected]>
matthewdale
added a commit
to matthewdale/mongo-go-driver
that referenced
this pull request
Oct 1, 2024
Co-authored-by: Steven Silvester <[email protected]>
matthewdale
added a commit
that referenced
this pull request
Oct 8, 2024
…ster] (#1842) Co-authored-by: Steven Silvester <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GODRIVER-3156
Summary
connection.idleDeadlinetoconnection.idleStart, allowing us to calculate different idle deadlines for different purposes.Background & Motivation
Currently the Go driver never attempts to detect connections that were closed by the other side when checking out connections. As a result, it's possible to return a connection that's been closed by the other side where any read or write operations will immediately fail. Instead, we should check if the connection has been closed when checking out a connection.